I like that you did this. It is more easily readable IMO. Currently, you're protected in all cases with the ifs. But hopefully someone later will know better to not trust that trx_id is set.

Good point, will address this in #1360 (id is cached inside transaction there)

To avoid using an uninitialized id, perhaps wrap it with optional? May cause some overheads though.

Although this is better than the old code to prevent duplicate transactions from being pushed, I think it's not 100% safe due to the use of now(). For example, when maximum_time_until_expiration is 1 day, if last blocks in local database is 1 day before, after finished replay, the dupe-trx db would be empty, so when pushing new blocks received from p2p, dupe check will always pass even if a duplicate transaction is in a block.

Ideally we should use the timestamp of head block after replayed as now, however according to the gap processing below, perhaps it's hard to get head before replay is finished.

Alternatively, we can skip dupe-check during replay, and revisit the pushed blocks to fill the dupe-check db just before enabling undo_db. In order to determine how many blocks to revisit, we can keep track of the greatest maximum_time_until_expiration appeared during replay (assuming it's T) and least block_interval appeared (assuming it's I), then blocks_to_revisit = T/I.

You are correct, but I think it doesn't matter. The dupe check is really only relevant when a witness signs a new block, and that happens only close to now().
If replay ends at now - 1 day, then either we receive blocks created in the past 24 hours and have their tx's added to the dupe database, or there's a gap in the blockchain and the next block will be generated at now() which means older tx's have already expired.

My point is it's not only about block generation but also about block validation. In case when witnesses colluded signing/accepting blocks with duplicate transactions, other nodes remain the ability to recognize the misbehavior and reject invalid blocks.

I agree practically it's not a big deal. We've been running without that check for years, due to restrict timing/conditions required to exploit the potential issue. E.G. it only affects nodes that just finished a replay, but during normal operations most nodes won't be in that state.

IMHO we can leave the logic as proposed in this PR, but better add some comments there explaining why we did so and what scenarios have been considered and why ignored.

After rethinking I switched to last_block->timestamp instead of now. After a chain halt (of course these won't happen again, so it doesn't really matter) it wouldn't require colluding witnesses - any witness could sneak a block with a dupe into the gap.
(Note that this would be noticed during a full sync.)